-
Notifications
You must be signed in to change notification settings - Fork 575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add matplotlib style configuration for circuit drawing #1811
Conversation
Hello. You may have forgotten to update the changelog!
|
Codecov Report
@@ Coverage Diff @@
## master #1811 +/- ##
=======================================
Coverage 98.81% 98.81%
=======================================
Files 226 227 +1
Lines 17233 17264 +31
=======================================
+ Hits 17028 17059 +31
Misses 205 205
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just some small things! Out of curiosity, can we have a black white and green style?
|
||
if __name__ == "__main__": | ||
black_white_style_example() | ||
black_white_style_dark_example() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a newline for the end of the file (formatting)
black_white_style_dark_example() | |
black_white_style_dark_example() | |
pennylane/circuit_drawer/styles.py
Outdated
except (ModuleNotFoundError, ImportError) as e: | ||
has_mpl = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice check,
Codecov is complaining that these lines aren't being tested. Not sure how you would test them since you don't have access to the environment during the test.
pennylane/circuit_drawer/styles.py
Outdated
|
||
""" | ||
|
||
if not has_mpl: # pragma: no cover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is pragma?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excludes the line for code coverage. I'll just have to add this above as well.
This module contains styles for using matplotlib graphics. | ||
|
||
To add a new style: | ||
* create a private function that modifies ``plt.rcParams``. | ||
* add an entry to the private dictionary ``_style_map``. | ||
* Add an entry to ``doc/code/qml.drawer.rst`` | ||
* Add a test in ``tests/drawer/test_style.py`` | ||
|
||
Use the decorator ``_needs_mpl`` on style functions to raise appropriate | ||
errors if ``matplotlib`` is not installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we place this information somewhere more user facing? Like in the docs somewhere ?
I think it would be helpful to developers who might just read the docs and not look at the source code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this information should be "user facing". If a developer (or external contributor) is wanting to create a new style, this should be the first file they look at anyway.
rcparams(circuit) | ||
Solarize_Light2(circuit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rip solarize_light2
pennylane/transforms/draw.py
Outdated
You can globally control the style with ``plt.rcParams`` and styles | ||
If we customize ``plt.rcParams``, we get a | ||
different style: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording sounded a little weird. Is styles a separate parameter used with plt.rcParams
? If not I think this suggestion sounds better:
You can globally control the style with ``plt.rcParams`` and styles | |
If we customize ``plt.rcParams``, we get a | |
different style: | |
You can globally control the style with ``plt.rcParams``. | |
If we customize ``plt.rcParams``, we get a different style: | |
assert plt.rcParams["lines.color"] == "black" | ||
assert plt.rcParams["text.color"] == "black" | ||
|
||
plt.style.use("default") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to reset it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want it interfering with potential other tests. These changes are global and so would affect later tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure !
assert plt.rcParams["lines.color"] == "white" | ||
assert plt.rcParams["text.color"] == "white" | ||
|
||
plt.style.use("default") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(same as above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just small documentation points, but other then that happy to approve
pennylane/drawer/tape_mpl.py
Outdated
You can also control the style with ``plt.rcParams`` and styles, see the | ||
`matplotlib docs <https://matplotlib.org/stable/tutorials/introductory/customizing.html>`_ . | ||
If we customize ``plt.rcParams``, we get a | ||
different style: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording sounded a little weird. Is styles a separate parameter used with plt.rcParams? If not I think this suggestion sounds better:
You can also control the style with ``plt.rcParams`` and styles, see the | |
`matplotlib docs <https://matplotlib.org/stable/tutorials/introductory/customizing.html>`_ . | |
If we customize ``plt.rcParams``, we get a | |
different style: | |
You can also control the style with ``plt.rcParams``, see the | |
`matplotlib docs <https://matplotlib.org/stable/tutorials/introductory/customizing.html>`_ . | |
If we customize ``plt.rcParams``, we get a different style: |
Co-authored-by: Jay Soni <jbsoni@uwaterloo.ca>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome looks good! Glad to have these styles in 👍🏼
[sc-12056] |
The new matplotlib circuit drawing characteristics make very few styling choices for users, instead relying on matplotlib configuration. This gives a great deal more flexibility for users to tune things to their own personal preferences.
But the default style doesn't always look the best, and individual fine tuning can take a lot of time and effort.
Therefore this PR adds (at least one) function that will modify
plt.rcParams
for preconfigured styles.Styles can be stored in
*.mplstyle
data files, but I do not know how to best ship pennylane with stylesheets in a convenient way. Therefore, this PR just altersplt.rcParams
directly.